Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: config route with management fileds (E2E) #927

Closed

Conversation

idbeta
Copy link
Contributor

@idbeta idbeta commented Nov 30, 2020

Please answer these questions before submitting a pull request

@idbeta idbeta changed the title test: config route with management fileds test: config route with management fileds (E2E) Nov 30, 2020
func TestRoute_with_name_desc(t *testing.T) {
tests := []HttpTestCase{
{
caseDesc: "config route with name and desc (r1)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the first letter lowercase of caseDesc?

Copy link
Contributor Author

@idbeta idbeta Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's unreasonable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@idbeta I think it can be regarded as "standard".

that is totally wrong. I created a related issue yesterday: #912
we'll fix this later.

caseDesc is not a good field name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I see, we can fix it in 2.2, what do you think? @nic-chen

"name": "jack",
"create_time": 1604046556,
"update_time": 1604046556,
"desc": "config route with name and desc",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad desc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just used to compare the case results, so it's good as long as it does not exist before.

Sleep: sleepTime,
},
{
caseDesc: "verify the route's detail (r1)",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this test case different from the first one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is to verify that input create_time, update_time can also create routes.

respBody, _ := ioutil.ReadAll(resp.Body)
createtime := gjson.Get(string(respBody), "data.create_time")
updatetime := gjson.Get(string(respBody), "data.update_time")
assert.NotEqual(t, createtime.String(), "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not compare the time here?

Copy link
Contributor Author

@idbeta idbeta Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, the timestamp should also be compared here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @moonming Can you review it when you have time?

Body: `{
"uri": "/hello",
"name": "jack",
"create_time": 1604046556,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should forbidden to specify create_time, update_time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if manager-api wants to support the command line to create routes in the future, it probably should not be forbidden to specify create_time, update_time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create_time and update_time are fields maintained by manager-api, and we do not need the caller to care about them.

so I think we can delete create_time and update_time here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a new issue, we can it later: #933

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I delete that part.

@codecov-io
Copy link

codecov-io commented Dec 1, 2020

Codecov Report

Merging #927 (25f5f62) into master (a7d8e13) will increase coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
+ Coverage   42.82%   43.10%   +0.27%     
==========================================
  Files          18       18              
  Lines        1289     1290       +1     
==========================================
+ Hits          552      556       +4     
+ Misses        645      642       -3     
  Partials       92       92              
Impacted Files Coverage Δ
api/internal/core/entity/format.go 60.46% <0.00%> (+8.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7d8e13...25f5f62. Read the comment docs.

@membphis
Copy link
Member

membphis commented Dec 1, 2020

@idbeta your branch is wrong, we can close this PR then you can submit a new one

@membphis membphis closed this Dec 1, 2020
@idbeta idbeta deleted the e2e-test-config-route-with-management-fileds branch December 9, 2020 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add e2e test for config route with management fileds
4 participants